Test: add more aggregation focused dictionary sql logic test#23280
Conversation
|
@Jefffrey could you take a look |
There was a problem hiding this comment.
@Rich-T-kid
Thanks! I took a look and this LGTM overall.
I just have one small suggestion that could make the test coverage a bit more robust.
| # multiple dictionary columns as group keys | ||
|
|
||
| statement ok | ||
| CREATE TABLE dict_multi_key AS |
There was a problem hiding this comment.
Nice addition. This expands the GROUP BY coverage for multiple dictionary columns well.
One thought for a future follow-up: all of the rows here come from a single VALUES-derived table, so this is mostly exercising one dictionary encoding per column. It could be useful to add a case where the same logical values arrive in different batches or inputs with different dictionary key assignments. That would help catch implementations that accidentally group by dictionary keys instead of decoded values.
Not blocking though. The current tests already cover multiple dictionary key columns, mixed key widths, nulls, and aggregates.
There was a problem hiding this comment.
thanks for the review @kosiew , I agree I think this would be a nice addition. Id be happy to make a follow up PR to add this.
3 participants
Which issue does this PR close?
works towards #22682.
Rationale for this change
There is a lack of testing for multi-dictionary group bys. It make sense to introduce these test before the implementation of
Dict<K,V>in #23187What changes are included in this PR?
introduces a couple test
Dict<_,largeutf8>dict<_,_>Are these changes tested?
the changes are test.
Are there any user-facing changes?
no